fix: apply FastEmbed query/passage asymmetry via query_embed - #1265
fix: apply FastEmbed query/passage asymmetry via query_embed#1265Aryan-Pardeshi wants to merge 1 commit into
Conversation
embed_query previously delegated to embed_documents, so asymmetric models like bge-small-en-v1.5 embedded queries through the passage path and lost the model's query-side instruction, degrading retrieval quality (basicmachines-co#1264). Route embed_query through TextEmbedding.query_embed, sharing the existing L2 normalization and dimension checks with embed_documents via a _normalize_vectors helper. Documents are unaffected, so stored vectors and identity keys stay valid. Closes basicmachines-co#1264
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e0ba28bdb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # silently loses that asymmetry for queries (#1264). query_embed takes no | ||
| # batch/parallel kwargs — it embeds a single query string. | ||
| def _embed_query() -> list[list[float]]: | ||
| return self._normalize_vectors(model.query_embed(text)) |
There was a problem hiding this comment.
For the default bge-small-en-v1.5 path this still embeds the raw query through the same ONNX embed path: in FastEmbed 0.8.0, TextEmbedding.query_embed only delegates to the selected model, and the base string implementation calls self.embed([query], **kwargs) (source, source). Therefore the #1264 scenario for the default FastEmbed model still produces the same vector as before, while the new stub test invents a distinct query vector; if the fix is meant to restore the BGE query role, this path needs to add/use the actual role prefix or otherwise exercise real FastEmbed behavior rather than just switching method names.
Useful? React with 👍 / 👎.
2 participants
Summary
FastEmbedEmbeddingProvider.embed_query()delegated toembed_documents(), so asymmetric models such asbge-small-en-v1.5embedded queries through the passage/document path and silently lost the model's query-side instruction — the retrieval-quality regression measured in #1264.Changes
embed_querynow routes throughTextEmbedding.query_embed(text)in a worker thread, which is FastEmbed's query path for asymmetric models.sqlite_search_repository's unit-vector distance contract) and the dimension mismatch check are shared withembed_documentsvia a_normalize_vectorshelper — both paths keep identical post-processing.embed_documentsbehavior, identity keys, and stored vectors stay valid, so no re-indexing is triggered.Notes
semantic_embedding_document_input_type/semantic_embedding_query_input_typeremain LiteLLM-only: FastEmbed has noinput_typerequest parameter — the role split isquery_embedvsembed, so no config plumbing is needed for the native provider.query_embedaccepts nobatch_size/parallelkwargs (single query string), so the runtime knobs stay on the document path only.Testing
query_embed(asserted distinct query vs passage vectors, and thatembedis never called), plus unit-norm and dimension-mismatch coverage for query vectors.tests/repository/suite + semantic embedding watch tests: 654 passed, 30 skipped.ruff check,ruff format --check, andty checkclean.Closes #1264